Skip to content

Add tlog_mirror crate with wire format and default ticket authenticator#236

Merged
lukevalenta merged 2 commits into
mainfrom
lvalenta/tlog-mirror-wire-format
Jul 14, 2026
Merged

Add tlog_mirror crate with wire format and default ticket authenticator#236
lukevalenta merged 2 commits into
mainfrom
lvalenta/tlog-mirror-wire-format

Conversation

@lukevalenta

@lukevalenta lukevalenta commented May 5, 2026

Copy link
Copy Markdown
Contributor

NOTE(lvalenta): I used Claude to help generate this PR, but have reviewed and approve the changes.

Introduces the tlog_mirror crate, implementing the wire-format pieces of c2sp.org/tlog-mirror.

@lukevalenta

Copy link
Copy Markdown
Contributor Author

/bonk review

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a careful review against the c2sp.org/tlog-mirror spec and ran the pre-push checks. Wire-format math, ticket scheme, and test coverage are solid. A few things to address before this lands:

Must-fix

1. Stale TicketSealer references break cargo doc (error.rs, wire.rs)

Three intra-doc links to crate::TicketSealer survive from an earlier AEAD-based design — the type is now TicketMacer. Confirmed locally:

$ RUSTDOCFLAGS=\"-D rustdoc::broken_intra_doc_links\" cargo doc -p tlog_mirror --no-deps
error: unresolved link to \`crate::TicketSealer\`
  --> crates/tlog_mirror/src/wire.rs:64:26
error: unresolved link to \`crate::TicketSealer\`
  --> crates/tlog_mirror/src/error.rs:62:41

Locations:

  • crates/tlog_mirror/src/wire.rs:64 — ``TicketSealer``
  • crates/tlog_mirror/src/error.rs:62 — `Errors returned by `TicketSealer`.`

While you're in error.rs, the TicketError variant docs are also AEAD-flavored leftovers that no longer match the implementation:

  • Line 65: "sealed ticket too short: ... 12-byte nonce" — there is no nonce; the threshold is TAG_LEN = 16.
  • Line 70: "AEAD authentication failed: either the ciphertext or the nonce has been tampered with" — no AEAD, no nonce, no ciphertext.

Suggest rewording to refer to TicketMacer, TAG_LEN, and HMAC tag verification.

2. cargo package -p tlog_mirror fails — missing README.md

crates/tlog_mirror/Cargo.toml:3 hard-codes readme = \"README.md\", but no README.md exists in the crate directory. Sibling crates (tlog_witness, tlog_cosignature, static_ct_api) use readme.workspace = true, which resolves to the workspace root README during packaging.

$ cargo package -p tlog_mirror --no-verify --allow-dirty --list
error: readme \`README.md\` does not appear to exist (relative to \`.../crates/tlog_mirror\`).

This will also block any future cargo publish. Easiest fix: change to readme.workspace = true, matching siblings.

3. Integer overflow in package_ranges iterator (wire.rs:185)

let next_boundary = (start / PACKAGE_ALIGNMENT + 1) * PACKAGE_ALIGNMENT;

This overflows u64 whenever start / 256 == u64::MAX / 256, i.e. roughly start >= 0xFFFF_FFFF_FFFF_FF00. In debug builds this panics with attempt to multiply with overflow; in release it wraps to 0, which makes end = next_boundary.min(upload_end) = 0, and since self.next = end = 0, the iterator silently restarts and yields wrong ranges.

Reachable from network input because AddEntriesRequestHeader::read_from accepts any upload_end up to u64::MAX. No real log will ever hit 2^64 entries, but a malicious or buggy peer absolutely can put any 8-byte value on the wire.

Suggested fix — saturate at upload_end:

let next_boundary = (start / PACKAGE_ALIGNMENT)
    .checked_add(1)
    .and_then(|q| q.checked_mul(PACKAGE_ALIGNMENT))
    .unwrap_or(self.upload_end);
let end = next_boundary.min(self.upload_end);

Add a regression test, e.g. package_ranges(u64::MAX - 1, u64::MAX) should yield (u64::MAX - 1, u64::MAX) and then terminate.

Should-fix

4. PR description claims publish = false but the manifest doesn't have it

The PR description says the crate is publish = false while the spec stabilizes, but crates/tlog_mirror/Cargo.toml has only [package.metadata.release] release = false — that's a cargo-release automation marker, not a cargo publish gate. (static_ct_api, which is explicitly published per AGENTS.md, has the same metadata.release block.) Either add publish = false under [package] or amend the PR description so it doesn't promise a guard that isn't there. Worth being explicit since the AGENTS.md "crates.io" boundary is an approval gate.

Nits / discussion

5. parse_decimal_u64 is stricter than the spec text

The spec says "in decimal" without explicitly forbidding leading zeros / + / whitespace. The current parser rejects all of those, which is fine for serializing — we only want one canonical form — but slightly aggressive when parsing a body produced by another mirror implementation. The inline comment already justifies this ("we want a single canonical encoding for each integer"), so I'm fine leaving it as-is, but worth flagging in case there's appetite for a Postel split (lenient parser, strict serializer).

6. Test gap (low priority)

EntryPackage::read_from converts num_entries: u64 to usize and only fails on 32-bit targets. Not realistically reachable from a tlog mirror, but a #[cfg(target_pointer_width = \"32\")] test is essentially free if you want to lock the behavior in.


Pre-push checks I ran locally on the head commit:

  • cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic
  • cargo test -p tlog_mirror ✅ (34/34)
  • cargo fmt --all --check
  • cargo doc -p tlog_mirror ❌ (broken intra-doc links — see #1)
  • cargo package -p tlog_mirror --no-verify --list ❌ (missing README — see #2)

@ask-bonk

ask-bonk Bot commented May 5, 2026

Copy link
Copy Markdown

Review submitted. Summary of what I posted on PR #236:

Must-fix (3):

  1. Broken doc linksTicketSealer references and AEAD/nonce verbiage in error.rs:62,65,70 and wire.rs:64 are leftovers from a prior design. cargo doc with -D rustdoc::broken_intra_doc_links fails.
  2. cargo package failsCargo.toml:3 hard-codes readme = "README.md" but the file doesn't exist; sibling crates use readme.workspace = true.
  3. Integer overflow in package_ranges(start / 256 + 1) * 256 panics or wraps when start >= u64::MAX - 254. Reachable from wire input. Suggested checked-arithmetic fix + regression test.

Should-fix (1): PR description says publish = false but the manifest only has cargo-release metadata, not a publish gate.

Discussion/nits (2): strict decimal parser is more strict than spec text (currently inline-justified; flagged for awareness); 32-bit target test gap (low priority).

Locally clippy/test/fmt all pass; only cargo doc and cargo package fail (per items 1 and 2). Submitted as a "request changes" review.

github run

@lukevalenta lukevalenta force-pushed the lvalenta/tlog-mirror-wire-format branch from e68442a to 789acbb Compare May 5, 2026 01:47
@lukevalenta

Copy link
Copy Markdown
Contributor Author

/bonk review again

@ask-bonk

ask-bonk Bot commented May 5, 2026

Copy link
Copy Markdown

@lukevalenta Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed against the previous review's findings and against the spec. All three must-fix items and the should-fix item are addressed; pre-push checks (including the two that previously failed) all pass locally on the head commit.

Previous review items — verified fixed

1. Stale TicketSealer references / AEAD verbiage

  • wire.rs:64 now points to crate::TicketMacer.
  • error.rs:62 rewords to [\TicketMacer`](crate::TicketMacer)`.
  • error.rs:65–75 rewords the variants to refer to TAG_LEN (16 bytes) and HMAC tag verification — no more "12-byte nonce" / "AEAD authentication failed".
  • RUSTDOCFLAGS="-D rustdoc::broken_intra_doc_links" cargo doc -p tlog_mirror --no-deps now passes.

The only remaining mention of "AEAD" in the crate is in ticket.rs:34, in the doc paragraph that explicitly invites operators to roll their own AEAD-based scheme — that's intentional, not a leftover.

2. Missing README breaks cargo package

  • crates/tlog_mirror/Cargo.toml:10 now uses readme.workspace = true, matching siblings.
  • cargo package -p tlog_mirror --no-verify --allow-dirty --list succeeds and includes README.md (resolved from the workspace root).

3. Integer overflow in package_ranges

  • wire.rs:189–192 now uses checked_add(1).and_then(|q| q.checked_mul(PACKAGE_ALIGNMENT)).unwrap_or(self.upload_end), with an inline comment explaining the saturation.
  • New regression test package_ranges_no_overflow_at_u64_max (wire.rs:540–553) covers both u64::MAX - 1, u64::MAX and edge, u64::MAX (where edge = u64::MAX - (u64::MAX % 256)). Both yield a single correct range and terminate.
  • Total tests: 35 (up from 34).

4. publish = false not in manifest

  • crates/tlog_mirror/Cargo.toml:5 now sets publish = false with a comment explaining it'll be lifted once the spec stabilizes. [package.metadata.release] release = false is also retained for cargo-release.

Pre-push checks (head commit 789acbb)

cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic   ✅
cargo test                                                                ✅ (full workspace green; tlog_mirror 35/35)
cargo fmt --all --check                                                   ✅
RUSTDOCFLAGS="-D rustdoc::broken_intra_doc_links" cargo doc -p tlog_mirror --no-deps  ✅
cargo package -p tlog_mirror --no-verify --allow-dirty --list             ✅

(cargo machete not installed in this sandbox; CI runs it.)

Tiny nit (non-blocking)

The PR body still says "34 unit tests covering …". With the overflow regression test added, it's 35. Worth a one-character tweak before merge if you're amending the description; otherwise no action needed.

LGTM — approving.

@ask-bonk

ask-bonk Bot commented May 5, 2026

Copy link
Copy Markdown

Re-review submitted as APPROVED on PR #236.

Summary of what I posted:

All four previously-flagged items are fixed on head commit 789acbb:

  1. TicketSealer doc links / AEAD verbiagewire.rs:64 and error.rs:62,65–75 now correctly reference TicketMacer/TAG_LEN/HMAC. cargo doc with -D rustdoc::broken_intra_doc_links passes.
  2. Missing READMECargo.toml:10 switched to readme.workspace = true. cargo package --list succeeds.
  3. Integer overflow in package_ranges — checked-arithmetic fix applied at wire.rs:189–192, with a regression test (package_ranges_no_overflow_at_u64_max) covering both u64::MAX - 1 and the alignment edge.
  4. publish = false claim — now set in Cargo.toml:5 with an explanatory comment.

All five pre-push checks (clippy, test, fmt, doc, package) pass locally; tests went from 34 to 35.

Only nit: PR description still says "34 unit tests"; it's 35 now. Non-blocking.

Approved.

github run

@lukevalenta lukevalenta marked this pull request as ready for review May 5, 2026 13:10
@lukevalenta lukevalenta self-assigned this May 5, 2026
@lukevalenta lukevalenta force-pushed the lvalenta/tlog-mirror-wire-format branch from 789acbb to d2978b2 Compare May 5, 2026 19:30
@lukevalenta lukevalenta removed the request for review from rozbb July 10, 2026 12:53
@lukevalenta lukevalenta force-pushed the lvalenta/tlog-mirror-wire-format branch 2 times, most recently from 499b2e0 to 21b0ffa Compare July 10, 2026 18:28
Implements the wire-format pieces of c2sp.org/tlog-mirror:

- `AddEntriesRequestHeader`: read/write framing for the add-entries
  request body header (u16 log_origin, u64 upload_start/end, u16
  ticket).
- `EntryPackage`: read/write framing for a single entry package
  (u16-prefixed entries + u8 num_hashes <= 63 + 32-byte hashes).
- `package_ranges`: deterministic iterator over the spec's
  256-aligned `[start, end)` intervals derived from upload_start
  and upload_end. Callers iterate this rather than recomputing the
  rounding math.
- `MirrorInfo`: read/write framing for the `text/x.tlog.mirror-info`
  409 Conflict response body (3 newline-terminated lines: tree size,
  next entry, base64-encoded ticket).
- `TicketSealer`: default opaque-ticket authenticator using
  deterministic AES-256-GCM-SIV over the payload, with the log origin
  as associated data. Encryption enforces the spec's ticket opacity so
  clients cannot depend on the internal format; confidentiality is not
  required because pending checkpoints are public.

The mirror's `add-checkpoint` endpoint reuses
`tlog_witness::add_checkpoint` per spec, so this crate does not
duplicate it.
@lukevalenta lukevalenta force-pushed the lvalenta/tlog-mirror-wire-format branch from 21b0ffa to 3c7e8b9 Compare July 10, 2026 18:29
@lukevalenta

Copy link
Copy Markdown
Contributor Author

/bonk review

@ask-bonk

ask-bonk Bot commented Jul 10, 2026

Copy link
Copy Markdown

APIError: Invalid Anthropic API Key

github run

2 similar comments
@ask-bonk

ask-bonk Bot commented Jul 10, 2026

Copy link
Copy Markdown

APIError: Invalid Anthropic API Key

github run

@ask-bonk

ask-bonk Bot commented Jul 10, 2026

Copy link
Copy Markdown

APIError: Invalid Anthropic API Key

github run

@ask-bonk

ask-bonk Bot commented Jul 10, 2026

Copy link
Copy Markdown

@lukevalenta Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@lukevalenta

Copy link
Copy Markdown
Contributor Author

/bonk review

@ask-bonk

ask-bonk Bot commented Jul 10, 2026

Copy link
Copy Markdown

LGTM – Approved

All previously identified issues have been addressed:

  1. Doc links & AEAD verbiage – Updated to reference TicketMacer, corrected TAG_LEN and HMAC wording. cargo doc now passes with -D rustdoc::broken_intra_doc_links.
  2. Missing README – Switched to readme.workspace = true; cargo package succeeds.
  3. Integer overflow in package_ranges – Implemented checked arithmetic with saturation and added regression test covering the edge case.
  4. Publish flag – Added publish = false to the crate manifest with explanatory comment.
  5. Test count – Updated test suite now reports 35 tests (non‑blocking nit).

All pre‑push checks (clippy, test, fmt, doc, package) pass locally. No remaining blockers. 🚀

github run

Comment thread Cargo.toml Outdated
Comment thread crates/tlog_mirror/src/ticket.rs Outdated
/// # Errors
/// [`ParseError::Io`] on short reads, or [`ParseError::TooManyHashes`]
/// if `num_hashes` exceeds 63.
pub fn read_from<R: Read>(mut reader: R, num_entries: u64) -> Result<Self, ParseError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specification limits each entry package to at most 256 entries, but read_from accepts any u64 and passes it to Vec::with_capacity. An incorrect count could cause a very large allocation or panic. Could we reject counts above PACKAGE_ALIGNMENT before allocating? It may also be worth rejecting packages with more than 256 entries in write_to, so the type cannot serialize an invalid package

Comment thread crates/tlog_mirror/src/wire.rs
Comment thread crates/tlog_mirror/src/wire.rs Outdated
@lukevalenta lukevalenta merged commit 74e14fa into main Jul 14, 2026
10 checks passed
@lukevalenta lukevalenta deleted the lvalenta/tlog-mirror-wire-format branch July 14, 2026 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants